Feature/ Helia React Vite Example #16
Conversation
polus-arcticus
changed the title
There was a problem hiding this comment.
LGTM, minor comments nits.
One thing I didn't post earlier is that imports should have newline between them and first line of code in the file.. but that's not a big deal. Readability is, but I don't think that's gonig to cause any future viewers issues, and we should have a lint rule set up if we want to enforce any line spacing.
| } else { | ||
| console.log('please wait for helia to start') | ||
| } | ||
| }, [error, starting, cid, helia]) // make sure to add helia in the dependencies array of the fs will suspend |
There was a problem hiding this comment.
grammatical error on the inline comment here makes it hard to understand. is it supposed to be something like
| }, [error, starting, cid, helia]) // make sure to add helia in the dependencies array of the fs will suspend | |
| }, [error, starting, cid, helia, fs]) // make sure to add helia in the dependencies array or helia fs will be unresponsive |
also, we should include all of the dependencies that are used within the callback
There was a problem hiding this comment.
https://github.com/polus-arcticus/helia-examples/blob/eaddd053efd4d1692701e0c33bbe9132513da303/examples/helia-vite-react/public/fs-dep-vs-no-dp-with-progress.gif
is a gif illustrating what I mean, I added it as i'm not sure if this behaviour from unixfs is expected
There was a problem hiding this comment.
}, [error, starting, cid, helia, fs])
// If one forgets to add helia in the dependency array in commitText, additions to the blockstore will not be picked up by react, leading to operations on fs to hang indefinitely in the generator <suspend> state. As such it would be good practice to ensure to include helia inside the dependency array of all hooks to tell react that the useCallback needs the most up to date helia state
return { cidString, commitedText, commitText, fetchCommitedText }
There was a problem hiding this comment.
that's a lot more context, above and beyond; thanks.
|
Thanks for the comments @SgtPooki 🎉 I'll make these changes |
examples/helia-vite-react/public/fs-dep-vs-no-dp-with-progress.gif
Outdated
Show resolved
Hide resolved
you bet, i probably just could've uploaded them to imgur or something |
|
@polus-arcticus thanks so much for the contribution!! If you want to help out more, there's a group of prioritized examples at ipfs/helia#43! just ping me on that issue if you want to grab one and I can put your name next to it so we can make sure any other contributors aren't stepping on toes. |
Thanks @SgtPooki ! okay sounds good 👍 |
Thought I'd try writing an example for Vite+React